Skip to content

Drupal 7 authentication#25

Closed
WardinT wants to merge 1 commit intonextcloud:masterfrom
WardinT:WardinT-drupal-1
Closed

Drupal 7 authentication#25
WardinT wants to merge 1 commit intonextcloud:masterfrom
WardinT:WardinT-drupal-1

Conversation

@WardinT
Copy link
Copy Markdown

@WardinT WardinT commented Feb 13, 2018

No description provided.

@mlojewski-me
Copy link
Copy Markdown
Collaborator

Thanks for the request but I don't think that's a good commit. I will loop up at this later, when I find some time. But it's definitely a good change.

@mlojewski-me
Copy link
Copy Markdown
Collaborator

Commit made.
Thanks for code delivery :)
ebcdaa8

@aanderse
Copy link
Copy Markdown

@mlojewski-me This code was removed in commit c1cc89f but the comment simply says "'lib' rewritten." Was it intentional to remove this code in that commit? Is there any chance of resurrecting this functionality?
@WardinT Do you still have any interest in this functionality?

@mlojewski-me
Copy link
Copy Markdown
Collaborator

I think 'Portable PHP password' is the same algorithm, but I'm not sure.

@BrandonKerr
Copy link
Copy Markdown
Contributor

BrandonKerr commented Oct 1, 2018

@mlojewski-me I reviewed the 'Portable PHP Password' option and found a few changes that needed to be made for it to work with Drupal 7. Drupal 7 uses sha512 instead of md5 and had a couple other changes as noted below:

@@ -75,15 +75,15 @@
             return null;
         }
 
-        $hash = md5($salt . $password, true);
+        $hash = hash('sha512', $salt . $password, true);
         do {
-            $hash = md5($hash . $password, true);
+            $hash = hash('sha512', $hash . $password, true);
         } while (--$count);
 
         $output = substr($setting, 0, 12);
-        $output .= $this->encode64($hash, 16);
+        $output .= $this->encode64($hash, strlen($hash));
 
-        return $output;
+        return substr($output, 0, 55);
     } 

How would you recommend bringing these changes back in? I assume other apps use Phpass as it is now. Would it be better to bring in a dedicated Drupal 7 option again, or add an option to Portal PHP password?

@mlojewski-me mlojewski-me reopened this Oct 1, 2018
@WardinT
Copy link
Copy Markdown
Author

WardinT commented Oct 1, 2018

I'd prefer the dedicated Drupal 7 option.

@BrandonKerr
Copy link
Copy Markdown
Contributor

I've worked up my changes as a separate file for a dedicated Drupal 7 option, which extends Phpass in order to be as unobtrusive as possible. Alternatively, we could update Phpass to take optional arguments, but I see that you have open issue #46 which sounds like it's along those same lines and I'd prefer to not get in your way for that.
Would it be best to fork and create a new pull request, or would you prefer a just adding the file here so you can work it into this existing PR (pending your approval, of course)?

@mlojewski-me
Copy link
Copy Markdown
Collaborator

I think issue #46 is going to take a long time to finish, so in my mind the best option is to make PR with dedicated class for Drupal 7, which extends Phpass.
When the PR is ready I will check it and approve.

@BrandonKerr
Copy link
Copy Markdown
Contributor

Thanks for the feedback. There's a new PR for your review now.

@mlojewski-me
Copy link
Copy Markdown
Collaborator

Moved to #66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants